-
Notifications
You must be signed in to change notification settings - Fork 37
Conversation
@wanderer o/ here is an update :) This is required me to make more changes than I hoped for, but things are getting ready and I'm feeling pretty happy about this "interface-ipld-format". If you want to start hacking the js-ipld-eth-block, check ipld/js-ipld-dag-pb#1, it is pretty much there. I added you as a contributor to https://github.com/ipld/js-ipld-eth-block. I'm going to continue and churn through the other things :) |
@nicola I don't know how much you have seen of this, but this is important re the discussions we had in tr past two week. It seems this changes everything we planned for, so we have to start over again to a degree. |
I don't think it's a drastic change, @diasdavid can you be in the call today? |
Not in general, but in regards to the abstractions for the implementation I think it changes some things. |
03755d2
to
0818945
Compare
Update to the interface-ipld-formatBy implementing dag-cbor, I realized that we only (and benefit from) having an interface for serialize, deserialize and cid detached from the format class. This enables raw json objects to be passed around like dag-cbor, without having to do an instance of a specific DAGNode cbor class. That being said, the new interface for an IPLD format resumes to:
Another way to view it is that the main resolver doesn't really care what type does the 'deserialize' call return. |
} | ||
|
||
// Adds support for an IPLD format | ||
// default ones are dag-pb and dag-cbor | ||
support (multicodec, type, resolver, util) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how do you know now if something is cbor or pb?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Through the CID
de3a8ee
to
a186055
Compare
@diasdavid one other consideration (maybe its already been considered, im just still trying to grok) is that merkle links and the DAG topology / traversal should be independent of each other. So for example, i would want to do something like
but underneath account doesn't have a true merkle link to balance. Its all part of the |
@wanderer you can achieve that in two ways:
we can go through this during today's js-ipfs chat :) |
Getting close to that IPLD Week 2 goal :) An excellent way to achieve that is by CR the following PR:
Please, if you review it, remove the 'Needs review' label, so that work doesn't get duplicated. //cc @victorbjelkholm @dignifiedquire @RichardLitt MOVED TABLE TO: ipfs/js-ipfs#532 (comment) |
} | ||
} | ||
|
||
resolve (cid, path, callback) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are cid
and path
two different inputs?
Can we not just pass cid
+path
?
the "deconcatenation" is something that I will happen to do often
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nicola note that CID is a instance of class CID that has several utility methods https://github.com/ipfs/js-cid/tree/feat/complete
What you are proposing is to use cids always as strings, which would assume that we have to be constantly parsing and mounting them again to get their props.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I see, perfect then.
There must be up in the levels of abstraction a sort of resolver that takes a path and resolve that (by using this, I guess)
|
||
// TODO: Consider if we still want this (fact: no one is using it | ||
// right now and this will be just an IPLD selector anyway | ||
getRecursive (cid, callback) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
During the Toronto's IPLD call, we mentioned that this should be a BFS and should have an optional parameter x
where if set to false
or -1
, we explore recursively the entire graph, otherwise, we just move with depth x
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What we talked was .tree
method inside an IPLD format.
This getRecursive
is an old method that comes from the previous "DAGService", which was never used in practice. This method returns the nodes directly, not their values.
What you make me think is that we should have a .tree
at the main IPLD Resolver and make that option part of the interface-ipld-format for local resolvers too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
^ perfect
|
||
[![](https://img.shields.io/badge/made%20by-Protocol%20Labs-blue.svg?style=flat-square)](http://ipn.io) | ||
[![](https://img.shields.io/badge/project-IPFS-blue.svg?style=flat-square)](http://ipfs.io/) | ||
[![](https://img.shields.io/badge/freenode-%23ipfs-blue.svg?style=flat-square)](http://webchat.freenode.net/?channels=%23ipfs) | ||
[![Coverage Status](https://coveralls.io/repos/github/ipfs/js-ipld-resolver/badge.svg?branch=master)](https://coveralls.io/github/ipfs/js-ipld-resolver?branch=master) | ||
[![Travis CI](https://travis-ci.org/ipfs/js-ipld-resolver.svg?branch=master)](https://travis-ci.org/ipfs/js-ipld-resolver) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's move this to the ipld organization as part of this effort
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, it's already on the IPLD org, forgot to change the badges. Good catch :)
"description": "IPLD implementation in JavaScript", | ||
"main": "lib/index.js", | ||
"description": "IPLD Resolver Implementation in JavaScript", | ||
"main": "src/index.js", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: remove before merge
const CID = require('cids') | ||
const until = require('async/until') | ||
const IPFSRepo = require('ipfs-repo') | ||
const MemoryStore = require('../node_modules/interface-pull-blob-store/lib/reference.js') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can do require('interface-pull-blob-store/lib/reference.js')
instead, no need for the full path
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right! (I do remember fighting with this one and this was how I got it to reference to the right one, not sure what was the error without node_modules though).
Could you also check ipfs-inactive/interface-pull-blob-store#6, this way we don't even have to point to a path inside.
|
||
// Support by default dag-pb and dag-cbor | ||
this.support(dagPB.resolver.multicodec, dagPB.resolver, dagPB.util) | ||
this.support(dagCBOR.resolver.multicodec, dagCBOR.resolver, dagCBOR.util) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should there be a way to disable these defaults?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about having functions to add and remove, so that:
this.support.add
this.support.rm
// Adds support for an IPLD format | ||
// default ones are dag-pb and dag-cbor | ||
support (multicodec, resolver, util) { | ||
this.resolvers[multicodec] = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe warn on overriding?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
if (err) { | ||
return cb(err) | ||
} | ||
const result = this.resolvers[cid.codec].resolver.resolve(block, path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there should be a check with a nice error message for this.resolvers[cid.codec]
as this can easily be undefined
) | ||
} | ||
|
||
// TODO: Consider if we still want this (fact: no one is using it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lets use this opportunity to drop the recursive methods
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 👍
/* | ||
* Test different types of data structures living together | ||
* & | ||
* Test data made of mixed data structures! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are you planning to fill this before merge?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes :) cbor + pb
@@ -0,0 +1,205 @@ | |||
/* eslint-env mocha */ | |||
'use strict' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the tests in cbor
and proto
are super similar, maybe you could extract the test methods, and just pass in the type agains it should test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar, but not the same, cbor is way more flexible and needs even more throughout tests. Creating a base tests would make every other format be tested very much like dag-pb, which is the less interesting data format because of its rigidity.
🎉 |
Following up the technical discussions pre and post DEVCON2, this PR (and all the listed PRs) update the interfaces and add support to the panoply of IPLD Formats to live and be resolved through IPFS, without breaking links nor requiring transformations (this is huge).
Introducing interface-ipld-format
The interface-ipld-format is a specification for a data type that enables the IPLD Resolver to manipulate and resolve through different types of Merkle Data Structures. The interface is very simple:
UPDATE to the interface: See: #60 (comment)
.serialize()
- returns a serialized version of this instance.deserialize(<data>)
- deserializes a binary blob into the instance.cid
- returns the CID of this instance.resolve(block, path)
- resolves a path in block, returns the value and or a link and the partial missing path. This way the IPLD Resolver can fetch the link and continue to resolve..tree(block, options)
- returns all the paths available in this blockA full specification of the interface will be defined at https://github.com/ipld/interface-ipld-format. Meanwhile, you can use https://github.com/ipld/js-ipld-dag-pb/tree/awesome-ipld as an example.
UPDATE to the interface: See: #60 (comment)
Tasks
Moving to IPLD and starting to use IPLD formats, required changes to be applied from IPFS-Repo and up, like full support of CID, multicodec spec update and impl, etc. Here are the list of tasks that need to be performed to make this transition complete, so that is easier to track state.